OBS-144: Migrate from socorro-release to obs-common #245
Conversation
relud
force-pushed
the
relud-obs-common-and-just
branch
2 times, most recently
from
ddf75c8
to
5d1cbe2
Compare
There was a problem hiding this comment.
Looks good to me!
In general, I'd prefer to have separate PRs for separate changes, since it makes the Git history more useful when trying to figure out when something changed and for what reason. I think it's fine here, and it's not worth the effort to try and factor this into separate, independent commits, so this is just a general remark.
We need some updates to the documentation as well. We should probably add a link to https://just.systems/ somewehere, update all references to make, and update references to the scripts now provided by obs-common.
I think this PR should also remove the scripts replaced by obs-common from this repo.
justfile
Outdated
|
|
||
| # Stop and remove docker containers and artifacts. | ||
| clean: | ||
| docker compose rm -f |
There was a problem hiding this comment.
Running docker compose rm only removes containers, but not networks, containers and images. Should we rename down to clean instead?
Networks specifically can be a problem on Linux, since they create entries in the routing table that often conflict with random wifi networks, so I generally want down rather than just rm to clean things up. (The first time I ran into this was on a German train. The wifi on German trains uses a 172.x.0.0/16 network, and so does Docker, and they often conflict, with the result that you can connect to the wifi, but no IP packets are ever sent there. This wasn't easy to debug.)
There was a problem hiding this comment.
I think I should make clean do docker compose down, but I would like to also keep down as a separate command, and add more things to clean, like this line from docs:
docker compose run --rm --no-deps eliot bash make -C docs/ clean
| docker compose --progress plain build {{args}} | ||
|
|
||
| # Run the webapp and services. | ||
| run *args='--attach=eliot --attach=fakesentry eliot': _env |
There was a problem hiding this comment.
It's confusing that the target running docker compose up is called run. Most other targets are named according to the Docker Compose command they run. Moreover, there's also docker compose run, which does something else. I'm not sure we should actually change this, since this has always been called run. I mainly want to hear what you think. :)
(There's also docker compose start, which is similar to docker compose up, except less useful in completely unclear ways. I like that we don't have a just target for that.)
There was a problem hiding this comment.
I think it's a great idea to rename these just commands to just up and just run to match docker compose
There was a problem hiding this comment.
per discussion in zoom, only just run should be moved to just up, and we'll keep just shell
There was a problem hiding this comment.
I'm going to file this change as a separate PR so willkg can have a chance to comment on it as well
| docker compose up {{args}} | ||
|
|
||
| # Stop service containers. | ||
| stop *args: |
There was a problem hiding this comment.
just stop is a very satisfying command to type into a shell.
|
Forgot to mention this explicitly: I tested all just targets manually on my Ubuntu machine, and everything appears to be working fine. |
relud
force-pushed
the
relud-obs-common-and-just
branch
from
5d1cbe2
to
64be446
Compare
and from make to just
relud
force-pushed
the
relud-obs-common-and-just
branch
from
64be446
to
79a2401
Compare
I've updated dev.rst, which was the only place I found references to make.
I didn't find any references to the 4 scripts moved to obs-common:
done |
and from make to just
also switch to using
docker-compose.override.ymlfor volume mounts, so they can be disabled in CI.